Skip to content

feat: add new collection type, BoundedVec, to the bounded collections crate#2199

Merged
DSharifi merged 16 commits intomainfrom
just-bounded-vec
Feb 20, 2026
Merged

feat: add new collection type, BoundedVec, to the bounded collections crate#2199
DSharifi merged 16 commits intomainfrom
just-bounded-vec

Conversation

@DSharifi
Copy link
Copy Markdown
Contributor

part of #2186

This PR is broken out of #2165, by only creating the bounded vec type, and not integrating it into the contract yet. See slack thread for motivation https://nearone.slack.com/archives/C0912BTG51T/p1771583540732579?thread_ts=1771583231.086569&cid=C0912BTG51T

@DSharifi DSharifi changed the title feat: add new collection type, BoundedVec, to the bounded collectiosn crate feat: add new collection type, BoundedVec, to the bounded collections crate Feb 20, 2026
@DSharifi DSharifi marked this pull request as ready for review February 20, 2026 10:57
@claude
Copy link
Copy Markdown

claude bot commented Feb 20, 2026

Code Review: BoundedVec addition

The crate rename and BoundedVec implementation look well-structured overall, with good test coverage (unit, serde, borsh, and schema tests). A few items to consider:

Potential issue: iter_mut can break invariants (low severity for now)

iter_mut() on BoundedVec cannot change the length, so the bounded invariant is preserved. However, worth noting for future maintainers that if the invariant ever depends on element values (not just count), iter_mut would be a bypass vector. For the current use case (length-only bounds), this is fine.

witnesses::possibly_empty doesn't validate U > 0 (minor)

witnesses::possibly_empty::<U>() at bounded_vec.rs:73 accepts U = 0, which would create a type that can only hold an empty vec. This is technically valid but may be surprising. The non_empty witness correctly validates L > 0 and L <= U. Consider adding a compile-time check U > 0 in possibly_empty if a zero-capacity vector is never intended, or document this edge case explicitly.

hex_serde length calculation could overflow for large U (minor)

At bounded_vec.rs:799-800, (L * 2) as u32 and (U * 2) as u32 will silently truncate if L or U exceed u32::MAX / 2. This only affects abi schema generation (not runtime correctness), and the bounds would need to be astronomically large, so it's not practically exploitable. Still, per CONTRIBUTING.md's safe arithmetic guidelines, u32::try_from(L.checked_mul(2)?) would be more defensive.

No blocking issues found. Good test coverage, clean crate rename, and the witness pattern provides solid compile-time safety.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most of the implementation is taken from https://github.com/ergoplatform/bounded-vec/blob/develop/src/bounded_vec.rs (license allows this).

Some small changes have been made, such as not implementing AsMut since that can break the invariants.

Borsh and scehma implementations have also been added for ABI generation.

gilcu3
gilcu3 previously approved these changes Feb 20, 2026
Copy link
Copy Markdown
Contributor

@gilcu3 gilcu3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

Left a few questions, mostly related to design choices

Copy link
Copy Markdown
Contributor

@SimonRastikian SimonRastikian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you

@DSharifi DSharifi added this pull request to the merge queue Feb 20, 2026
Merged via the queue into main with commit 5f9db4d Feb 20, 2026
10 checks passed
@DSharifi DSharifi deleted the just-bounded-vec branch February 20, 2026 18:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants